Add settings toggles for chart series (#1297)#2054
Add settings toggles for chart series (#1297)#2054dennisguse merged 1 commit intoOpenTracksApp:mainfrom
Conversation
grobolom
changed the title
dennisguse
left a comment
dennisguse
left a comment
There was a problem hiding this comment.
Nice!
Some feedback included.
Just one more thing: can you make all methods package private that you introduce?
I guess most of them should not be used outside of .chart.
src/main/java/de/dennisguse/opentracks/chart/ChartFragment.java
Outdated
Show resolved
Hide resolved
| boolean showElevation = PreferencesUtils.shouldShowElevation(); | ||
| if (showElevation != viewBinding.chartView.getShowElevation()) { | ||
| viewBinding.chartView.setShowElevation(showElevation); | ||
| viewBinding.chartView.applyShowElevation(); |
There was a problem hiding this comment.
Why do we need applyShowElevation()?
This can be done directly in setShowElevation().
This affects also the other two applyXXX-methods you introduced.
There was a problem hiding this comment.
@dennisguse I was thinking it would make sense that the settter method only modifies the value itself, while apply could handle any other kind of processing that is related to the current value. But I'm not opposed to rolling it in - I was mostly following the existing convention for applyReportSpeed.
|
@dennisguse thank you for the feedback! I'll update, and I'm going to give adding some tests for the new functionality a shot, if I can. I had a question - how do y'all handle translations? I see most pull requests that modify files like |
|
@dennisguse one more question - did you mean the new methods in |
|
Translations: handled via Weblate. Just add the english ones and the rest will be translated eventually if somebody feels the need for it. Package private: yes. Regarding the code: if you see something that can be improved, you can just do it. |
| } | ||
| boolean getShowPaceOrSpeed() { return showPaceOrSpeed; } | ||
| void setShowHeartRate(boolean value) { | ||
| showHeartRate = value; |
There was a problem hiding this comment.
You can use isEnabled() on each Series: no need to store this value here also.
There was a problem hiding this comment.
@dennisguse while this is true for showHeartRate and showElevation, the boolean tracking is necessary for showPaceOrSpeed because of the interaction between the two elements on the Chart itself. If you prefer, I can remove it for just the elevation and heart rate, but I could imagine someone returning and needing it again, if they work on this code further.
|
@dennisguse ready for one more round I believe - tests are Red but they seem to be the same ones as the last release. Manual testing on API version 35 as well as my Pixel 3a shows no issues that I can find. Also, please let me know if you'd like me to squash my commits, I am happy to if that leaves a cleaner commit path. |
dennisguse
left a comment
dennisguse
left a comment
There was a problem hiding this comment.
Looks good.
And yes, there are two tests prone to failure.
No easy way to fix it :/
|
PS squashing commits would be great. |
Adds toggles to the User Interface section of the app Settings that allow the user to display or hide the three series available in the chart: Elevation, Speed/Pace, and Heart Rate. As a user of a heart rate monitor primarily during strength workouts and combat sports, the distance and elevation graphs are not helpful to me - in addition, their scale means that I am not able to easily see my exact heart rate. This should allow users in a similar situation to better determine exactly what is most helpful for them to be seen on the graph. Also updates heart rate series possible intervals - the smallest interval was 25, which was too low for most zone 1 cardio charts - if your heart rate goes from 70 to 125, with a 25 interval, most of the chart will be whitespace.
grobolom
force-pushed
the
addSettingsTogglesForChartSeries#1297
branch
from
862cfa8 to
39c993f
Compare
|
@dennisguse squashed and pushed :) |
grobolom
changed the title
2 participants
Description
Adds toggles to the User Interface section of the app Settings that allow the user to display or hide the three series available in the chart: Elevation, Speed/Pace, and Heart Rate. As a user of a heart rate monitor primarily during strength workouts and combat sports, the distance and elevation graphs are not helpful to me - in addition, their scale means that I am not able to easily see my exact heart rate. This should allow users in a similar situation to better determine exactly what is most helpful for them to be seen on the graph.
Related Issues